Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added custom hashers support for HashSet and HashMap. #808

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Added custom hashers support for HashSet and HashMap. #808

merged 2 commits into from
Sep 20, 2023

Conversation

s3rius
Copy link
Contributor

@s3rius s3rius commented Sep 9, 2023 •
edited
Loading

Hello. The library is super great, but I noticed one problem. Custom hashers are not supported.

I introduced some new generics that allow you to serialize HashMaps and HashSets with custom hashers.

I wanted to speedup my code and have chosen ahash as custom hasher for my hashmaps. But found out that I need to convert hashmap back to default hasher in order to serialize it to valueslist.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@s3rius s3rius force-pushed the feature/custom-hashers branch from 59f465d to cea41c0 Compare September 9, 2023 18:44
@piodul
Copy link
Collaborator

piodul commented Sep 11, 2023

The min_rust check failed due to one of our indirect dependencies bumping up its MSRV. We'll fix that soon.

piodul
piodul requested changes Sep 11, 2023
View reviewed changes
Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

It looks like you also need to modify the impl on line cql_to_rust.rs:203 so that deserialization of hashmaps with custom hashers is also possible.

Apart from that, two more nitpicks but LGTM otherwise.

scylla-cql/src/frame/value.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/value.rs Show resolved Hide resolved
@piodul
Copy link
Collaborator

piodul commented Sep 12, 2023

The issue with failing min_rust check was fixed in #810. Rebase on main will be needed.

@s3rius s3rius force-pushed the feature/custom-hashers branch from cea41c0 to 30c6e12 Compare September 12, 2023 20:01
@s3rius s3rius requested a review from piodul September 15, 2023 16:12
@piodul
Copy link
Collaborator

piodul commented Sep 20, 2023

The test if_lwt_optimisation_mark_offered_then_negotiatied_and_lwt_routed_optimally failed twice and only succeeded now in the third run. It's not related to the PR at all so I'll merge, but I'll open an issue about this test being flaky.

@piodul piodul merged commit ceba60a into scylladb:main Sep 20, 2023
1 check passed
@piodul piodul mentioned this pull request Sep 20, 2023
Open
@s3rius
Copy link
Contributor Author

s3rius commented Sep 21, 2023

Thank you very much. Have a nice day.

@s3rius s3rius deleted the feature/custom-hashers branch September 21, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piodul piodul Awaiting requested review from piodul

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@s3rius @piodul